-
-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFC 0057: Configuration metadata API #57
base: main
Are you sure you want to change the base?
Conversation
Overview of definitions.
Expand on the specific metadata settings contain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far! Unfortunately, I'm not well-versed enough in the technical side of things to critique possible implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This spec certainly covers a lot of the bases, but I feel that it overcomplicates some things while leaving others out.
FREE_TEXT, | ||
SLIDER, | ||
NUMERIC, | ||
UNORDERED_COLLECTION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All config values supported by loaders config API are inherently ordered. Since the behavior of an unordered collection is undefined and I struggle to think of when any of the possible behaviors might be useful, I don't think it really needs to be included.
#### Widget Type | ||
|
||
```java | ||
public enum WidgetType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think supporting custom widget types is important. If somebody wants to implement a color wheel widget for their config option, they should be able to do so. Even if their widget doesn't appear alongside other widgets, a solution that opens another screen to edit the option would seem reasonable to me. I think restricting widgets to a small set of types will lead to some less-than-optimal end user experiences.
|
||
### Definitions | ||
- **Setting**: A single value and its associated metadata. | ||
- **Category**: A logical grouping of Settings. A Setting may be a member of multiple Categories. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love the term "category" here. I would ordinarily assume that each setting belongs to only one category, similar to how game rules have a single category or biomes have a single category. I'm not sure what a better word would be, but wanted to open the discussion.
ORDERED_COLLECTION; | ||
} | ||
|
||
MetadataType<WidgetType, ?> WIDGET_TYPE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's important that no matter what our system ends up being, we provide reasonable default behaviors for any type. This seems like it would be a good place to note that.
If a Setting's Widget Type is `UNORDERED_COLLECTION` or `ORDERED_COLLECTION`, additional data is stored `WIDGET_ELEMENT_TYPE`. | ||
|
||
```java | ||
MetadataType<WidgetType, ?> WIDGET_ELEMENT_TYPE; | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is necessary at all. We should have one standard way of displaying collections, and collections should simply define the widget type used by all of their children.
MetadataType<WidgetType, ?> WIDGET_ELEMENT_TYPE; | ||
``` | ||
|
||
#### Value Converter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't think this is necessary. WIDGET_TYPE
should just be a MetadataType<Function<TrackedValue<?>, Widget>>
, with different kinds of widgets having either constructors or helper methods for different parameter types. ToggleWidget
for instance could have the following method:
public static Widget create(TrackedValue<?> value) {
Object defaultValue = value.value();
if (defaultValue instanceof Boolean) {
return new ToggleWidget(value.value());
} else if (defaultValue instanceof Enum e && e.values().length == 2) {
return new EnumToggleWidget(value.value());
} else if (...) {
} else {
throw new WidgetCreationException(...); // Might be better to have some sort of result class instead of throwing errors, but ¯\_(ツ)_/¯
}
}
The value would then simply call
builder.metadata(WIDGET_TYPE, builder -> builder.factory(ToggleWidget::create))
#### Environment Policy | ||
|
||
```java | ||
public enum EnvironmentPolicy { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CLIENT_ONLY
and SERVER_ONLY
are functionally identical. I would therefore rename this class to SyncPolicy
or something along those lines and condense those two options to NONE
, with SERVER_SYNCED
becoming S2C
and CLIENT_SYNCED
becoming C2S
. Additionally, I might add a P2P
sync type that shares user configs with other clients in a way that allows them to be accessed on a per-player basis. This has a lot of valuable implications for user-set display information (nicknames come to mind).
Rendered